-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Documentation: Add support for deprecated/experimental messages #81458
Documentation: Add support for deprecated/experimental messages #81458
Conversation
This would be a breaking change for the XML format. We should find a way to avoid that. |
We can use two attributes, bool and string, just like in |
962a286
to
402f0a8
Compare
I love this, we've been generating generic messages like I noticed there were some empty deprecation messages. Should we try to avoid empty messages or is it intended that some messages will be empty? We can still provide the generic message in C# in these cases, but I think it'd be better to provide a better message. I'm not suggesting to fill all the messages in this PR, it could be a follow-up. I'm not sure how useful it is for experimental API though, it seems all the messages are empty in this PR. I also don't know what we would write here other than a generic |
@@ -1,5 +1,5 @@ | |||
<?xml version="1.0" encoding="UTF-8" ?> | |||
<class name="SkeletonIK3D" inherits="Node" is_deprecated="true" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="../class.xsd"> | |||
<class name="SkeletonIK3D" inherits="Node" deprecated="" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="../class.xsd"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there was someone asking recently why this class was deprecated, worried that it will be removed without an alternative. Maybe we could add a message here that clarifies that a replacement will be provided in the future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CC @godotengine/animation
I think so, but I've left the messages empty for now. We should also think about default messages. Now I've added a neutral "This ... may be changed or removed in future versions.", but this should probably be different for Deprecated and Experimental and for engine and user code.
Probably yes, although I think that sometimes we can indicate problems when using the experimental API: |
@@ -273,8 +289,13 @@ | |||
</xs:sequence> | |||
<xs:attribute type="xs:string" name="name" /> | |||
<xs:attribute type="xs:string" name="inherits" /> | |||
<!-- deprecated --> | |||
<xs:attribute type="xs:float" name="version" use="optional" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
402f0a8
to
4fde31e
Compare
deprecated_prefix = translate("Deprecated:") | ||
if item.deprecated.strip() == "": | ||
default_message = translate(f"This {item.definition_name} may be changed or removed in future versions.") | ||
result += f"**{deprecated_prefix}** {default_message}\n\n" | ||
else: | ||
result += f"**{deprecated_prefix}** {format_text_block(item.deprecated.strip(), item, state)}\n\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rST doesn't support arbitrary colors (aside of raw HTML tags). You could use the .. danger::
adominition though, which appears in red.
See also #63079.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4fde31e
to
0e04038
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM style and code wise
cbd43ce
to
bf9b2a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally (rebased on top of master
f317cc7), it mostly works as expected.
I can't get the deprecated/experimental reason messages to show up though:
extends Node2D
## Description.
##@deprecated: Test reason
var var1
##@experimental: Test reason 2
var var2
# Test comment (not a docblock).
##@deprecated: Test reason 3
func one() -> void:
pass
##@experimental: Test reason 4
##Description.
func two() -> void:
pass
@Calinou Looks like you were testing an older version: |
53d338a
to
eea19d6
Compare
I have completed self-review and testing. This initially small PR grew into a significant
|
eea19d6
to
513a46f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me overall!
@@ -1,5 +1,5 @@ | |||
<?xml version="1.0" encoding="UTF-8" ?> | |||
<class name="SkeletonIK3D" inherits="Node" is_deprecated="true" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="../class.xsd"> | |||
<class name="SkeletonIK3D" inherits="Node" deprecated="" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="../class.xsd"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CC @godotengine/animation
We should probably find some consensus on how to documentation deprecated and experimental methods/properties, and apply it throughout the codebase (cc @godotengine/documentation). Notably, how to document a method/property when it's deprecated because:
We can now put information in the deprecated message, but there's still the main description to deal with. What should it contain? Should it be kept empty if all the relevant info is in the deprecation message? If so, should we change |
I was taking a look. In my opinion, to make it extremely short:
Also these messages are not translatable now, are they? |
Yes, after merging this PR, some changes are needed in extract_classes.py. As for the docs, I agree with you, but I think that in any case we will need a follow-up PR for adjustments. However, I'm ready to apply suggestions that we are already confident about. |
I don't mind scouting across the class reference a second time as some form of standardization, post-merge. I got nothing better to do... |
513a46f
to
af28f87
Compare
Thanks! |
Add support for deprecated/experimental messages to core and GDScript. See:
@deprecated
annotation godot-proposals#7622Bugsquad edit, fixes: "Deprecated" needs to be more visible in the documentation godot-docs#7900
Production edit: closes godotengine/godot-roadmap#71